Introduce Scope to Dig #305

sywhang · 2021-12-08T07:44:07Z

This introduces the concept of Dig.Scope, which is a scoped container for modifications to the dependency graph. A Scope may have one or more Scopes as children. A Scope also has one parent Scope, except for Container, which is the root Scope. This change is necessary before we can start working on APIs for graph modifications and decorations.

Things to note:

Modifications to a Scope is "private" by default. For example, Provide to a Scope means it will be made available to itself and any of its descendent Scopes (More on this later).
Container, which is the existing API for DI container, is the just the root Scope.

To make things easier to review, I have broken down the PRs into few parts:

Introduce Dig.Scope type (this PR)
Add exported provide option to Provide, which will allow Provides to a Scope to be propagated to all the Scopes without having to touch the root Scope.
Add Decorate method to Dig.Scope, which lets you decorate a type provided to a Scope.

Notes on Implementation:

Each Scope struct keeps track of the following things:

constructorNodes, providers, groups, and values all track things that were provided directly to this Scope. It does not contain anything that was provided by its parent, child, or anything in its ancestry tree. This is so that we can avoid having to copy over these every single time a new Scope is created, which is expensive in terms of memory and computation.
gh still holds all the nodes in the entire dependency graph for nodes that impact this Scope. This is so that we can avoid re-computing the dependency graph every single time a graph algorithm needs to be run.

Ref: GO-939

…unctional

codecov · 2021-12-08T07:44:54Z

Codecov Report

Merging #305 (5b172dd) into master (d83e159) will decrease coverage by 0.09%.
The diff coverage is 98.84%.

@@            Coverage Diff             @@
##           master     #305      +/-   ##
==========================================
- Coverage   98.54%   98.45%   -0.10%     
==========================================
  Files          18       19       +1     
  Lines        1172     1228      +56     
==========================================
+ Hits         1155     1209      +54     
- Misses         10       11       +1     
- Partials        7        8       +1

Impacted Files	Coverage Δ
param.go	`98.01% <93.75%> (-0.43%)`	⬇️
scope.go	`98.66% <98.66%> (ø)`
constructor.go	`100.00% <100.00%> (ø)`
container.go	`100.00% <100.00%> (ø)`
cycle_error.go	`100.00% <100.00%> (ø)`
graph.go	`90.90% <100.00%> (ø)`
invoke.go	`100.00% <100.00%> (ø)`
provide.go	`100.00% <100.00%> (ø)`
visualize.go	`89.74% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aba513b...5b172dd. Read the comment docs.

sywhang · 2021-12-08T09:40:32Z

Marking this as WIP because I want to play around with an alternate implementation for Scope.Scope() where I don't have to copy all the providers, values, and groups each time a new Scope is created. But other than that, other parts (such as tests, Container -> Scope renames) are ready to review.

…cope name

param.go

r-hang · 2021-12-15T18:25:42Z

provide.go

+// same types are requested multiple times, the previously produced value will
+// be reused.
+//
+// In addition to accepting constructors that accept dependencies as separate


More succinct rewording:

Provide accepts argument types or dig.In structs as dependencies as well as separate return values or dig.Out structs for results.

provide.go

scope.go

scope_test.go

provide.go

abhinav

Gave it a high level first pass. Will give it another go later.
I think with the current implementation, invokes will ignore legitimate errors from constructors?

dig_test.go

scope.go

abhinav · 2021-12-17T19:48:47Z

constructor.go

 	}
-	n.order = c.newGraphNode(n)
+	s.newGraphNode(n, n.orders)


Passing n.orders into newGraphNode feels like a leak of internal state here,
but I can't think of an obvious alternative to this just yet.

scope.go

param.go

scope.go

param.go

container.go

scope.go

shirchen · 2021-12-21T23:28:09Z

scope.go

+// String representation of the entire Scope
+func (s *Scope) String() string {
+	b := &bytes.Buffer{}
+	fmt.Fprintln(b, "nodes: {")


nit, but thought i would call it out: fmt.Fprintln can return an error which isn't handled here.

yea it's a valid point, though we're writing it to a bytes.Buffer and not some external stream, and doing err check for every single Fprintln in this func can be expensive + hurts readability. i can add it though if you think the tradeoff is worth.

nah, i think you are just adding existing code. just thought i would mention in case it comes up again.

FWIW bytes.Buffer.Write always returns a nil error, so checking errors here would be unnecessary. I don't personally use errcheck much, but it will also ignore this case when the writer is known to be a bytes.Buffer at compile time.

scope_test.go

cycle_error.go

scope.go

scope_test.go

shirchen

lgtm. talked offline about adding permutations for tests for options that could interfere with this diff eg DryRun(true) and DeferAcyclicVerification(). Maybe there are others I didn't think of. I think its fine not to do that to existing ones, but for new ones, let's try and test more options.

… DryRun

cycle_error.go

abhinav · 2022-01-03T23:22:17Z

param.go

+		_, isErrMissingTypes := err.(errMissingTypes)
+		_, isErrMissingDeps := err.(errMissingDependencies)
+		if err != nil && !isErrMissingTypes && !isErrMissingDeps {


I'm not sure I understand what's going here fully:
err is assigned in a loop above. The value of err here will be the last thing assigned to it before the loop exits.
So this will report an error only if the a constructor in the middle somewhere failed, but one of the ones after it succeeded.

Is there risk here in implementing the scope-resolution logic at such a high level in the parameter tree?
If we implemented it on the leaf nodes (paramSingle and maybe paramGroupedSlice), we might be able to control how these operate in a multi-scope environment. Like, paramSingle.Build could call getValueProviders on current containerStore, then its parent, and so on until it finds a provider or runs out of stores.

abhinav · 2022-01-03T23:34:20Z

scope.go

+
+func newScope() *Scope {
+	s := &Scope{
+		name:      "container",


Two things:

Is there a reason we went with "container" as the name of the root node instead of empty string?

Since the name must always be set and is the only thing that changes, should this perhaps be a parameter (newScope(name))?

abhinav · 2022-01-03T23:37:42Z

scope.go

+func (s *Scope) appendLeafScopes(dest []*Scope) []*Scope {
+	dest = append(dest, s)
+	for _, cs := range s.childScopes {
+		dest = cs.appendLeafScopes(dest)
+	}


I don't recall if we already discussed naming for this operation.
"leaf scopes" seems inaccurate; these are all nodes in the subtree rooted at this node.
Perhaps "appendSubtree" would be more appropriate?

abhinav · 2022-01-03T23:38:52Z

scope.go

+	var stores []containerStore
+	for s := s; s != nil; s = s.parentScope {
+		stores = append(stores, s)
+	}
+	for i, j := 0, len(stores)-1; i < j; i, j = i+1, j-1 {
+		stores[i], stores[j] = stores[j], stores[i]
+	}
+	return stores


This is getScopesFromRoot but with a different type.
We can probably just call getScopesFromRoot, and then build a slice casting those elements:

scopes := getScopesFromRoot() stores := make([]containerStore, len(scope)) for i, s := range scopes { stores[i] = s }

abhinav · 2022-01-03T23:45:19Z

scope.go

+// String representation of the entire Scope
+func (s *Scope) String() string {
+	b := &bytes.Buffer{}
+	fmt.Fprintln(b, "nodes: {")


FWIW bytes.Buffer.Write always returns a nil error, so checking errors here would be unnecessary. I don't personally use errcheck much, but it will also ignore this case when the writer is known to be a bytes.Buffer at compile time.

This contains some miscellaneous fixes from additional feedback from uber-go#305. Notably: - Change err handling logic during scoped parameter building to be more clear on what it's doing. - Rename appendLeafScopes to appendSubscopes. - Change getStoresFromRoot to use getScopesFromRoot.

* Misc. fixes from #305 This contains some miscellaneous fixes from additional feedback from #305. Notably: - Make provider resolution for parameter building iterate the other way around (from self to root) and break once a provider is found. - Rename appendLeafScopes to appendSubscopes. - Rename getScopesFromRoot to ancestors - Change getStoresFromRoot to use storesToRoot. Co-authored-by: Abhinav Gupta <mail@abhinavg.net> Co-authored-by: Abhinav Gupta <abg@uber.com>

This adds fx.Module Option which is a first-class object for supporting scoped operations on dependencies. A Module can consist of zero or more fx.Options. By default, Provides to a Module is provided to the entire App, but there is a room for adding an option to scope that to a Module. Module can wrap Options such asSupply/Extract, Provide, and Invoke but there are some Options that don't make sense to put under Module. For example, StartTimeout, StopTimeout, WithLogger explicitly errors out when supplied to a Module. Implementation-wise, a Module corresponds to dig.Scope which was added in uber-go/dig#305. Extra bookkeeping is done by the module struct which contains the provides and invokes to a Scope. Co-authored-by: Abhinav Gupta <mail@abhinavg.net> Co-authored-by: Abhinav Gupta <abg@uber.com> Co-authored-by: Abhinav Gupta <mail@abhinavg.net>

* fx.Annotate: make variadic params optional by default If annotate is passed a variadic function, the dependency listed as the variadic parameter should be optional unless otherwise specified with the `optional:false` parameter tag. This commit addresses that bug. * Update annotated_test.go Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com> * fx.Annotate: make variadic params optional by default If annotate is passed a variadic function, the dependency listed as the variadic parameter should be optional unless otherwise specified with the `optional:false` parameter tag. This commit addresses that bug. * Update annotated_test.go Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com> * Temporarily pin Dig dependency to master (#827) This temporarily pins the Dig dependency in Fx to the master branch which has dig.Scope in preparation for adding fx.Module which is the corresponding user-facing API in Fx. In addition, this fixes a few tests to expect the new error message format that was changed with the graph refactoring PR in uber-go/dig#301. * Add fx.Module (#830) This adds fx.Module Option which is a first-class object for supporting scoped operations on dependencies. A Module can consist of zero or more fx.Options. By default, Provides to a Module is provided to the entire App, but there is a room for adding an option to scope that to a Module. Module can wrap Options such asSupply/Extract, Provide, and Invoke but there are some Options that don't make sense to put under Module. For example, StartTimeout, StopTimeout, WithLogger explicitly errors out when supplied to a Module. Implementation-wise, a Module corresponds to dig.Scope which was added in uber-go/dig#305. Extra bookkeeping is done by the module struct which contains the provides and invokes to a Scope. Co-authored-by: Abhinav Gupta <mail@abhinavg.net> Co-authored-by: Abhinav Gupta <abg@uber.com> Co-authored-by: Abhinav Gupta <mail@abhinavg.net> * fx.Module: Reorganize code (#832) In #830, I reverted some of the code moves to make reviewing the change easier. This change adds back those moves on top of that PR. This reverts commit e4d006b. This reverts commit 8aa68c0. Co-authored-by: Abhinav Gupta <abg@uber.com> * Finish incomplete merge * Only make variadic params optional if no other tags are specified * remove print statements * Update annotated.go Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com> * add extra test * Fix test "Provide variadic function with no optional params" * Explain why we're adding the optional tag * Fix test case, verified failing on master Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com> Co-authored-by: Sung Yoon Whang <sungyoon@uber.com> Co-authored-by: Abhinav Gupta <mail@abhinavg.net> Co-authored-by: Abhinav Gupta <abg@uber.com>

This adds fx.Module Option which is a first-class object for supporting scoped operations on dependencies. A Module can consist of zero or more fx.Options. By default, Provides to a Module is provided to the entire App, but there is a room for adding an option to scope that to a Module. Module can wrap Options such asSupply/Extract, Provide, and Invoke but there are some Options that don't make sense to put under Module. For example, StartTimeout, StopTimeout, WithLogger explicitly errors out when supplied to a Module. Implementation-wise, a Module corresponds to dig.Scope which was added in uber-go/dig#305. Extra bookkeeping is done by the module struct which contains the provides and invokes to a Scope. Co-authored-by: Abhinav Gupta <mail@abhinavg.net> Co-authored-by: Abhinav Gupta <abg@uber.com> Co-authored-by: Abhinav Gupta <mail@abhinavg.net>

* fx.Annotate: make variadic params optional by default If annotate is passed a variadic function, the dependency listed as the variadic parameter should be optional unless otherwise specified with the `optional:false` parameter tag. This commit addresses that bug. * Update annotated_test.go Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com> * fx.Annotate: make variadic params optional by default If annotate is passed a variadic function, the dependency listed as the variadic parameter should be optional unless otherwise specified with the `optional:false` parameter tag. This commit addresses that bug. * Update annotated_test.go Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com> * Temporarily pin Dig dependency to master (#827) This temporarily pins the Dig dependency in Fx to the master branch which has dig.Scope in preparation for adding fx.Module which is the corresponding user-facing API in Fx. In addition, this fixes a few tests to expect the new error message format that was changed with the graph refactoring PR in uber-go/dig#301. * Add fx.Module (#830) This adds fx.Module Option which is a first-class object for supporting scoped operations on dependencies. A Module can consist of zero or more fx.Options. By default, Provides to a Module is provided to the entire App, but there is a room for adding an option to scope that to a Module. Module can wrap Options such asSupply/Extract, Provide, and Invoke but there are some Options that don't make sense to put under Module. For example, StartTimeout, StopTimeout, WithLogger explicitly errors out when supplied to a Module. Implementation-wise, a Module corresponds to dig.Scope which was added in uber-go/dig#305. Extra bookkeeping is done by the module struct which contains the provides and invokes to a Scope. Co-authored-by: Abhinav Gupta <mail@abhinavg.net> Co-authored-by: Abhinav Gupta <abg@uber.com> Co-authored-by: Abhinav Gupta <mail@abhinavg.net> * fx.Module: Reorganize code (#832) In #830, I reverted some of the code moves to make reviewing the change easier. This change adds back those moves on top of that PR. This reverts commit e4d006b9cb4eb6eede8e1ca672cb4d74f679ddf2. This reverts commit 8aa68c04a85ac15123bdbaffa70ce7dac478660e. Co-authored-by: Abhinav Gupta <abg@uber.com> * Finish incomplete merge * Only make variadic params optional if no other tags are specified * remove print statements * Update annotated.go Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com> * add extra test * Fix test "Provide variadic function with no optional params" * Explain why we're adding the optional tag * Fix test case, verified failing on master Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com> Co-authored-by: Sung Yoon Whang <sungyoon@uber.com> Co-authored-by: Abhinav Gupta <mail@abhinavg.net> Co-authored-by: Abhinav Gupta <abg@uber.com>

sywhang and others added 6 commits December 6, 2021 12:47

add dig.Scope

339b30a

Merge remote-tracking branch 'upstream/master' into scope-1

5988a24

tests are all passing

06ffea5

add more tests, private provides and cross-Scope cycle checks fully f…

df3e502

…unctional

more tests

4d4df2f

make linter happy

374f584

sywhang added 2 commits December 9, 2021 22:57

avoid copying over providers,value,groups for creating child scope

5dc219e

each ctor gets provided exactly once; change err message to include s…

7e82f37

…cope name

sywhang changed the title ~~[WIP] Introduce Scope to Dig~~ Introduce Scope to Dig Dec 10, 2021

sywhang and others added 3 commits December 10, 2021 03:00

update comments

b531800

save/restore state for all Scopes affected

050ec0c

loop literal

487391d

r-hang reviewed Dec 15, 2021

View reviewed changes

abhinav mentioned this pull request Dec 16, 2021

[Feature Request] OverrideSupply and OverrideProvide uber-go/fx#825

Closed

moisesvega reviewed Dec 17, 2021

View reviewed changes

provide.go Outdated Show resolved Hide resolved

abhinav reviewed Dec 17, 2021

View reviewed changes

address code review feedback

cbf60a8

moisesvega approved these changes Dec 21, 2021

View reviewed changes

r-hang reviewed Dec 21, 2021

View reviewed changes

param.go Outdated Show resolved Hide resolved

typo

0ba222a

r-hang approved these changes Dec 21, 2021

View reviewed changes

container.go Outdated Show resolved Hide resolved

scope.go Outdated Show resolved Hide resolved

fix docstrings

9c35896

shirchen reviewed Dec 22, 2021

View reviewed changes

scope_test.go Outdated Show resolved Hide resolved

address code review feedback

2a86ead

shirchen approved these changes Dec 22, 2021

View reviewed changes

Add more permutations to scope test with DeferAcyclicVerification and…

5b172dd

… DryRun

sywhang merged commit cd97524 into uber-go:master Dec 22, 2021

sywhang deleted the scope-1 branch December 22, 2021 01:51

abhinav reviewed Jan 3, 2022

View reviewed changes

abhinav mentioned this pull request Jan 3, 2022

scope: Use an empty name for root #310

Merged

sywhang mentioned this pull request Jan 5, 2022

Misc. fixes from #305 #311

Merged

sywhang mentioned this pull request Feb 2, 2022

Add fx.Module uber-go/fx#830

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Scope to Dig #305

Introduce Scope to Dig #305

sywhang commented Dec 8, 2021 •

edited

Loading

codecov bot commented Dec 8, 2021 •

edited

Loading

sywhang commented Dec 8, 2021

r-hang Dec 15, 2021

abhinav left a comment •

edited

Loading

abhinav Dec 17, 2021

shirchen Dec 21, 2021

sywhang Dec 22, 2021

shirchen Dec 22, 2021

abhinav Jan 3, 2022

shirchen left a comment

abhinav Jan 3, 2022

abhinav Jan 3, 2022

abhinav Jan 3, 2022

abhinav Jan 3, 2022

abhinav Jan 3, 2022

Introduce Scope to Dig #305

Introduce Scope to Dig #305

Conversation

sywhang commented Dec 8, 2021 • edited Loading

codecov bot commented Dec 8, 2021 • edited Loading

Codecov Report

sywhang commented Dec 8, 2021

Choose a reason for hiding this comment

abhinav left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shirchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sywhang commented Dec 8, 2021 •

edited

Loading

codecov bot commented Dec 8, 2021 •

edited

Loading

abhinav left a comment •

edited

Loading